feat(studio): flat inspector foundation + Text group#2120
Conversation
vanceingalls
changed the title
miga-heygen
left a comment
miga-heygen
left a comment
There was a problem hiding this comment.
Reviewed as part of the 15-PR edit-panel redesign stack. Full stack review posted on #2120. No blockers on this PR. — Miga
miga-heygen
left a comment
miga-heygen
left a comment
There was a problem hiding this comment.
Full Stack Review — Edit Panel Redesign (15 PRs, ~10,700 additions)
Reviewed the entire stack in three tiers: foundation (#2120–#2122), domain groups + retirement (#2123–#2127), and UX polish (#2128–#2190). Per-PR verdicts below; no blockers across the stack.
Tier 1: Foundation (#2120–#2122)
#2120 — flat inspector foundation + Text group — Clean. Introduces the shell (header, footer, accordion, value-tier system, primitives) behind STUDIO_FLAT_INSPECTOR_ENABLED. resolveValueTier is a well-designed single decision point. hideOwnHeading flag on TextSection prevents doubled headings without duplication.
#2121 — Style group — Clean. Each sub-block (Fill, Stroke, Radius, Shadow, Blend, Blur, Overflow, Mask, Opacity) owns exactly one CSS domain. Helpers come from propertyPanelHelpers.ts (single source). FlatRadiusRow reuses legacy BorderRadiusEditor for non-uniform corners — good reuse.
#2122 — Layout + Motion groups — Clean. The standout SSOT contribution of the stack: deriveElementTiming extracts the "how do we compute start/duration?" decision into one function consumed by both Layout's keyframe gutter and Motion's timing row. Fixes a real bug where Layout used elDuration ?? 1 while Motion inferred from tweens.
Tier 2: Domain Groups + Retirement (#2123–#2127)
#2123 — Media group — Clean. Legacy MediaSection fully replaced, not duplicated. FlatToggle is a genuine new primitive.
#2124 — Grade (color grading) — Nit. The useColorGradingController extraction is the strongest SSOT win in the stack — turns a 200-line inline state machine into a shared hook. During the hybrid window both legacy and flat sections instantiate the hook (two debounce timers, two polling cascades). Resolved by #2126.
#2125 — persisted pinning + multi-field Text — Clean. FlatGroupDescriptor[] data model eliminates per-group copy-paste. usePersistedPinnedGroups is a clean single-path persistence hook.
#2126 — retire legacy Style and Grade — Clean. Pure SSOT cleanup — removes the dual-rendering transition state. The hideFlex escape hatch is gone.
#2127 — parity gaps from legacy-retirement audit — Clean. visibleIntensity helper centralizes the "revive from 0% strength" decision. Radius path simplified by always delegating to BorderRadiusEditor. Stroke width clamping reuses existing normalizePanelPxValue. Keyboard accessibility for compare hold button is thorough.
Tier 3: UX Polish (#2128–#2190)
#2128 — sticky accordion headers — Nit. Dual sticky anchoring (top-0 bottom-0) is unconventional but intentional per the design.
#2134 — scroll refactor — Clean. Well-structured three-region layout (before-open, scrollable body, after-open). FlatGroup → FlatGroupHeader rename correctly reflects narrowed responsibility.
#2135 — remove section pinning — Clean. Complete disciplined removal — no orphaned props, no dead code.
#2136 — animate expand/collapse — Clean. justToggledIds mechanism handles the Chromium re-trigger edge case thoughtfully. Respects prefers-reduced-motion. Well-commented.
#2142 — widen slider hit area — Clean. 2px → 20px invisible hit area. Minimal, correct fix.
#2186 — slider drag support — Clean. Pointer capture API is the correct modern approach. Properly layered before the debounce PR.
#2190 — debounce slider drag commits — Clean. Draft/debounce pattern gives instant visual feedback while debouncing commits at 40ms. Flush-on-release ensures final value always commits immediately. Broad test updates across sibling sections show thoroughness.
Cross-Stack Architecture
The stack follows a disciplined pattern:
- Flag-gated parallel path — flat inspector runs alongside legacy, gated by
STUDIO_FLAT_INSPECTOR_ENABLED. No legacy behavior changes. - Suppression flags over removal — when a flat group absorbs a legacy section, the legacy gets
hideOwnHeading/hideFlexrather than being removed, enabling safe coexistence. Retired cleanly in #2126. - Helpers carry new knowledge; primitives carry new rendering — every helper justifies itself with a fact the caller doesn't know. No trivial wrappers.
- Self-correcting SSOT — #2122 identifies and fixes the timing-basis disagreement that would have been an SSOT violation across Layout and Motion. The stack improves as it progresses.
CI: 0 failures across all 15 PRs. Ship the stack.
— Miga
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Blocker on current head 444639d: GitHub reports this PR as CONFLICTING / mergeState DIRTY. Resolve the branch against its stack/base and rerun the required checks before approval. The code review findings are non-blocking, but this head is not mergeable as submitted.
vanceingalls
force-pushed
the
studio-flat-01-foundation-text
branch
from
444639d to
6e9d99e
Compare
|
Rebased the stack base onto latest |
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Re-review after rebase: current head 6e9d99ef8f3aec054aebd05c9b2201f15aec4665 is mergeable, required checks have no failures/pending checks, and prior conflict blocker is resolved. No unresolved review threads; foundation/value-tier and cross-stack contracts remain clean.
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Superseding earlier approval: the latest max-review pass found confirmed correctness issues in this flat-inspector stack (identity/selector-index state, hide-all write races, timing inference, slider pointer/keyboard/reset semantics, and duplicate React keys). Hold merge and require fixes plus re-review on the current stack head.
vanceingalls
force-pushed
the
studio-flat-01-foundation-text
branch
from
6e9d99e to
535a072
Compare
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 535a072917cafb6d3940b914b175faf5b2602814. The foundation still has direct correctness blockers:
StudioRightPanel.tsx:346-349passeselement.id ?? element.selectorto a lookup whose canonical timeline identity includes source scope and selector occurrence. “Hide all” can silently miss its targets. The same loop starts whole-file read/modify/write operations concurrently, so same-file selections can clobber one another.PropertyPanel.tsx:265-273andPropertyPanelEmptyState.tsx:104-109omit source,hfId, andselectorIndexfrom React identity, allowing repeated-selector siblings to reuse stale panel state and duplicate list keys.- Flat Text still bypasses the legacy
PromotableControlpath for bound content/color/font fields. - Current CI is not green: File size check reports
StudioRightPanel.tsxat 603 lines (limit 600).
Use the canonical DOM/timeline target identity everywhere, batch/serialize the hide mutation into one save, restore variable-binding parity, and add repeated-selector/same-file regression tests.
Verdict: REQUEST CHANGES
Reasoning: Core element identity is noncanonical and Hide All can either no-op or lose updates; the head also has a required CI failure.
— Deepwork
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Additive adversarial re-review at exact head . The earlier identity, Hide All, variable-parity, and file-size blockers remain unchanged.\n\nStrength: keeps each choice as a native button, so basic focus/click behavior is preserved ().\n\n**[important] Glyph-only segmented choices have no accessible name or selected-state semantics.** exposes only , , and , but the button renders the glyph without , , or radio semantics (). The Text consumers discard their declared alignment labels and render , , plus two visually identical buttons for normal vs italic (). A screen reader cannot identify the choices or announce which one is selected. Add an accessible option label and pressed/radiogroup semantics, with a regression for the two choices.\n\nVerdict: REQUEST CHANGES\nReasoning: The current head is byte-identical to the prior requested-changes head and still contains those blockers; this pass adds an accessibility contract gap in the shared segmented primitive.\n\n— Deepwork
miguel-heygen
dismissed
their stale review
Formatting was corrupted during submission; replacing with corrected review.
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Additive adversarial re-review at exact head 535a072917cafb6d3940b914b175faf5b2602814. The earlier identity, Hide All, variable-parity, and file-size blockers remain unchanged.
Strength: FlatSegmentedRow keeps each choice as a native button, preserving basic focus behavior (propertyPanelFlatPrimitives.tsx:108-129).
[important] Glyph-only segmented choices lack accessible names and selected-state semantics. FlatSegmentOption exposes only key, node, and active, while the button has no aria-label, aria-pressed, or radio semantics (propertyPanelFlatPrimitives.tsx:87-129). Text renders L/C/R/J, AG/ag/Ag, and two indistinguishable A buttons for normal versus italic (propertyPanelFlatTextSection.tsx:27-39,151-181). Add accessible option labels and pressed or radiogroup semantics, with a regression for the two A choices.
Verdict: REQUEST CHANGES
Reasoning: This head is byte-identical to the prior requested-changes head and still contains those blockers; this pass adds an accessibility contract gap in the shared segmented primitive.
— Deepwork
…ld, TextAreaField
…AT_INSPECTOR_ENABLED Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…eading in multi-field fallback The flat inspector's Text FlatGroup rendered unconditionally, showing an empty "Text" header for non-text elements (image, video, etc). Gate it on isTextEditableSelection(element) so it disappears entirely when there's no text to edit. Also, the legacy multi-field TextSection (used as a fallback when an element has 2+ text fields) rendered its own internal "Text" heading nested inside the new flat Text FlatGroup, producing a doubled "Text" heading. Add a hideOwnHeading prop to TextSection (default false, so its other — legacy, non-flat — call site is unaffected) and pass it from FlatTextSection's fallback path.
vanceingalls
force-pushed
the
studio-flat-01-foundation-text
branch
from
535a072 to
7b5d8c7
Compare
|
Status on this review's findings, checked against the current stack tip: Fixed here — PR #2416, commit
Already fixed downstream, in PR #2225 (stacked above this PR, so it doesn't show at this PR's own head):
The file-size note is presumably about this PR's own diff size — no action taken there since it's outside the scope of the adversarial-review findings. Re-review whenever convenient — nothing outstanding from this pass that isn't already addressed somewhere in the stack. |
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Re-reviewed at 7b5d8c7d44264ea4345763d1af69a028fb14a1e1.
git range-diff marks all 12 incremental commits patch-equivalent to the previously reviewed head, so none of the requested fixes landed in this force-push. The existing identity/duplicate-selector, variable-promotion, Hide All lost-update, and accessibility findings therefore remain. In particular, StudioRightPanel.tsx:346-349 still launches independent read-modify-write visibility toggles, and propertyPanelFlatPrimitives.tsx:108-130 still gives glyph-only segmented buttons neither accessible option names nor selected-state semantics.
Fresh adversarial pass also found that FlatSelectRow disables only its <select> while leaving the reset button active (propertyPanelFlatPrimitives.tsx:423-454), so disabled Style rows can still mutate through their reset callbacks. CI is red: StudioRightPanel.tsx is 603 lines (limit 600), and Windows tests fail in the browser install-lock concurrency test.
Verdict: REQUEST CHANGES
Reasoning: This is a mechanical rebase, not a fix round; the prior correctness blockers and two required CI failures remain at the exact current head.
— Deepwork
|
Confirming: this PR's own head is unchanged, correctly — the fixes for this review's findings were not squashed backward into it. They land in PR #2225 and PR #2416, which sit directly on top of this PR in the same Graphite stack ( This is a stacked-PR review gate question, not a missing-fix question: the stack is merged bottom-up in one sitting via Graphite ( Evaluating each intermediate head against "is this correct if it were the final merged state" doesn't match how the stack actually ships. Could you review mergeability at the stack tip (#2416, which contains every fix) rather than blocking each intermediate PR on findings that are already resolved a few commits further up the same stack? Happy to walk through the specific commit that fixes any finding you want re-verified at the tip. |
A fresh full-stack re-review checked 15 PR heads independently. Cross- checked all 9 remaining claims against the actual current tip: - #2120 (id/selector key qualification, Hide All no-op/race, variable parity), #2121 (opacity-zero fallback), #2122 (GSAP preview sibling resolution, scrub-label), #2124/#2126 (negative metadata cache), and the keyboard-access half of #2121/#2186 were all already fixed by a later commit in this same stack (65954c3, PR #2225) — the reviewed heads predate it. Verified each in the current source rather than taking the isolated-head review at face value. - #2186's "no ESC/right-click cancel during drag" was the one claim that held up: FlatSlider had keyboard arrow-key support but no way to abort an in-progress pointer drag. Escape now reverts to the pre-drag value and releases pointer capture; a right-click (contextmenu) during a drag does the same instead of committing whatever position the pointer last reached while the native context menu opens over the slider. Both go through commitDraft (not just a visual reset) since the drag's leading- edge commit in onPointerDown may already have applied an intermediate value that needs actually undoing, not just hiding. propertyPanelFlatPrimitives.tsx crossed the 600-line file-size gate after this change; extracted FlatSelectRow into its own file, matching the FlatToggle/FlatMaskInsetRows precedent from earlier in this stack. New regression tests for Escape-cancel and contextmenu-cancel. Full studio suite still at the known pre-existing 55-failure baseline, zero new regressions. Typecheck/oxlint/oxfmt clean.
|
Re-checked all 5 claims against the current code (not just this PR's own head, since this is a Graphite stack that merges together — see the same explanation on earlier rounds). Verified each individually rather than taking either the "still broken" claim or my own assumption at face value:
All five are resolved in the current stack tip; nothing left to fix in this PR specifically. |
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Stack-scoped re-review at 7b5d8c7d44264ea4345763d1af69a028fb14a1e1.
The incremental foundation layer is unchanged, and its prior requested changes are now resolved in the accepted mandatory tip fix #2416 at 832d5be8e2ea0cc2144bb0486a25a0f7601e2ea1. Identity, Hide All, variable parity, accessibility, and file-size blockers are closed in the cumulative tip with regression coverage. This approval assumes the Graphite stack lands bottom-up through #2416. All required checks, including both Windows lanes, are green at this head.
Verdict: APPROVE
Reasoning: The accepted tip-fix strategy closes the foundation blockers, and required CI is green.
— Deepwork
…he flat-inspector stack Fixes issues raised in the Deepwork re-review of #2120-#2190 that weren't covered by #2225's earlier fix pass: - useColorGradingController: reset grading/compare/mediaMetadata state (and cancel pending persist/status timers) when selection changes to a different element — this hook is called unconditionally on every render (unlike legacy ColorGradingSection, remounted via a selectionIdentityKey React key), so switching selection reused the previous element's state. - useColorGradingController: stop permanently caching a non-OK /media/metadata response as null — a transient server error poisoned the HDR banner for that asset for the whole page lifetime. - FlatSelectRow: preserve a valid authored value outside the preset list (e.g. mix-blend-mode: difference, an arbitrary object-position) instead of silently misrepresenting it as the first preset — touching the control would overwrite real persisted state. - FlatSlider: the throttled trailing commit now reads onCommit through a ref updated every render instead of closing over it at schedule time — a caller whose onCommit spreads other current state (Grade's per-detail commits) could otherwise have a delayed commit revert whatever the user changed on a different control in the same 40ms window. - FlatSlider: flush a still-queued trailing commit on unmount instead of dropping it, and disable the reset button when the slider itself is disabled. - FlatSlider: add touch-action: none to the track so touch drags don't compete with page scroll. - FlatColorGradingAccessory: clean up the compare-hold's window listeners on unmount, not only on release — switching selection mid-hold used to leak them. - Align (flat Text): re-clicking the option already visually active for a logical start/end value no longer rewrites it to the physical left/right, preserving RTL semantics. - FlatSegmentedRow: give every option an accessible name and aria-pressed state — two visually-identical glyph buttons (upright/italic "A") had no way to be told apart by assistive tech. - PropertyPanelFlat: the panel body falls back to its own scroll when the collapsed group headers alone exceed the available height, so groups can't become permanently unreachable in a short pane. New regression tests for all of the above; full studio suite at the known pre-existing baseline (55 failures unrelated to this stack).
A fresh full-stack re-review checked 15 PR heads independently. Cross- checked all 9 remaining claims against the actual current tip: - #2120 (id/selector key qualification, Hide All no-op/race, variable parity), #2121 (opacity-zero fallback), #2122 (GSAP preview sibling resolution, scrub-label), #2124/#2126 (negative metadata cache), and the keyboard-access half of #2121/#2186 were all already fixed by a later commit in this same stack (65954c3, PR #2225) — the reviewed heads predate it. Verified each in the current source rather than taking the isolated-head review at face value. - #2186's "no ESC/right-click cancel during drag" was the one claim that held up: FlatSlider had keyboard arrow-key support but no way to abort an in-progress pointer drag. Escape now reverts to the pre-drag value and releases pointer capture; a right-click (contextmenu) during a drag does the same instead of committing whatever position the pointer last reached while the native context menu opens over the slider. Both go through commitDraft (not just a visual reset) since the drag's leading- edge commit in onPointerDown may already have applied an intermediate value that needs actually undoing, not just hiding. propertyPanelFlatPrimitives.tsx crossed the 600-line file-size gate after this change; extracted FlatSelectRow into its own file, matching the FlatToggle/FlatMaskInsetRows precedent from earlier in this stack. New regression tests for Escape-cancel and contextmenu-cancel. Full studio suite still at the known pre-existing 55-failure baseline, zero new regressions. Typecheck/oxlint/oxfmt clean.
…he flat-inspector stack Fixes issues raised in the Deepwork re-review of #2120-#2190 that weren't covered by #2225's earlier fix pass: - useColorGradingController: reset grading/compare/mediaMetadata state (and cancel pending persist/status timers) when selection changes to a different element — this hook is called unconditionally on every render (unlike legacy ColorGradingSection, remounted via a selectionIdentityKey React key), so switching selection reused the previous element's state. - useColorGradingController: stop permanently caching a non-OK /media/metadata response as null — a transient server error poisoned the HDR banner for that asset for the whole page lifetime. - FlatSelectRow: preserve a valid authored value outside the preset list (e.g. mix-blend-mode: difference, an arbitrary object-position) instead of silently misrepresenting it as the first preset — touching the control would overwrite real persisted state. - FlatSlider: the throttled trailing commit now reads onCommit through a ref updated every render instead of closing over it at schedule time — a caller whose onCommit spreads other current state (Grade's per-detail commits) could otherwise have a delayed commit revert whatever the user changed on a different control in the same 40ms window. - FlatSlider: flush a still-queued trailing commit on unmount instead of dropping it, and disable the reset button when the slider itself is disabled. - FlatSlider: add touch-action: none to the track so touch drags don't compete with page scroll. - FlatColorGradingAccessory: clean up the compare-hold's window listeners on unmount, not only on release — switching selection mid-hold used to leak them. - Align (flat Text): re-clicking the option already visually active for a logical start/end value no longer rewrites it to the physical left/right, preserving RTL semantics. - FlatSegmentedRow: give every option an accessible name and aria-pressed state — two visually-identical glyph buttons (upright/italic "A") had no way to be told apart by assistive tech. - PropertyPanelFlat: the panel body falls back to its own scroll when the collapsed group headers alone exceed the available height, so groups can't become permanently unreachable in a short pane. New regression tests for all of the above; full studio suite at the known pre-existing baseline (55 failures unrelated to this stack).
A fresh full-stack re-review checked 15 PR heads independently. Cross- checked all 9 remaining claims against the actual current tip: - #2120 (id/selector key qualification, Hide All no-op/race, variable parity), #2121 (opacity-zero fallback), #2122 (GSAP preview sibling resolution, scrub-label), #2124/#2126 (negative metadata cache), and the keyboard-access half of #2121/#2186 were all already fixed by a later commit in this same stack (65954c3, PR #2225) — the reviewed heads predate it. Verified each in the current source rather than taking the isolated-head review at face value. - #2186's "no ESC/right-click cancel during drag" was the one claim that held up: FlatSlider had keyboard arrow-key support but no way to abort an in-progress pointer drag. Escape now reverts to the pre-drag value and releases pointer capture; a right-click (contextmenu) during a drag does the same instead of committing whatever position the pointer last reached while the native context menu opens over the slider. Both go through commitDraft (not just a visual reset) since the drag's leading- edge commit in onPointerDown may already have applied an intermediate value that needs actually undoing, not just hiding. propertyPanelFlatPrimitives.tsx crossed the 600-line file-size gate after this change; extracted FlatSelectRow into its own file, matching the FlatToggle/FlatMaskInsetRows precedent from earlier in this stack. New regression tests for Escape-cancel and contextmenu-cancel. Full studio suite still at the known pre-existing 55-failure baseline, zero new regressions. Typecheck/oxlint/oxfmt clean.
3 participants
What
First PR in a 6-PR stack migrating Studio's right-panel property inspector from an always-expanded stacked-sections layout to a "flat" one-open-at-a-time accordion. This PR lays the foundation: the
STUDIO_FLAT_INSPECTOR_ENABLEDfeature flag, the accordion primitives (FlatRow,FlatSegmentedRow,FlatGroup,PinnedZoneDivider), the flat identity header/footer, and the first migrated group — Text.Stack: #2120 (this) → #2121 (Style) → #2122 (Layout+Motion) → #2123 (Media) → #2124 (Grade) → #2125 (Pinning + multi-field Text).
Why
The legacy inspector renders every applicable section expanded at once, which gets unwieldy as an element accumulates properties across style/layout/motion/media/grade. The flat redesign shows one section at a time (plus pinned sections), matching a design handoff mock.
How
FlatGroupowns the one-open accordion state (openGroupId/onToggleOpen) and pin affordance (onTogglePin), styled per the design mock.FlatTextSectionis the first migrated group and the reference implementation every later group's task followed for theisOpen/onToggleOpen/onTogglePin/summarywiring pattern.FlatGroupwas rendering unconditionally regardless of element type (empty for non-text elements), and the multi-field fallback doubled the "Text" heading. Fixed by gating onisTextEditableSelectionand adding ahideOwnHeadingprop to the legacyTextSectionfallback.STUDIO_FLAT_INSPECTOR_ENABLED(default off) — the legacy panel is untouched and remains the default for all users.Test plan
oxlint/oxfmtclean; this repo'sfallowcomplexity/duplication gate passes.